-
Notifications
You must be signed in to change notification settings - Fork 147
fix: ignore getBy inside of within for prefer-presence-queries on abs… #717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again @nathanmmiller! Thanks for adding the extra test. Let's add tests to cover all combinations:
Checking absence:
- with(getBy).getBy() --> invalid (added already)
- with(getBy).queryBy() --> valid (added already)
- with(queryBy).getBy() --> invalid
- with(queryBy).queryBy() --> valid
Checking presence:
- with(getBy).getBy() --> valid
- with(getBy).queryBy() --> invalid
- with(queryBy).getBy() --> valid
- with(queryBy).queryBy() --> invalid
@Belco90 Hm now that I look at this list, I think I disagree with the implementation (which might mean more work for me) but I don't think "within(queryBy)..." is valid in either case. Surely "within" should be treated much the same way as a presence query? Thoughts? I'm happy to add the 6 tests above - which will pass - but in seeing you write them out, I think that it's wrong that they should pass and I should only ignore within() if the inside is getBy. |
@nathanmmiller I think those examples and reporting the latest query chained from the Maybe it's easier to see with this example: const modal = screen.getByRole('dialog')
const { queryByRole } = within(modal)
expect(queryByRole('button', { name: 'submit' })).toBeInTheDocument() We need to report that |
So this makes perfect sense to me and I completely agree - I am not worried about that use case, I believe it's already covered by my original test (in the form of expect(within(blah).queryBy*).toBeIn - being wrong). What I'm saying is that I think So I believe my initial PR is wrong and if you agree, I will add more comprehensive unit tests to cover this as well, and modify it so that it's reported correctly. In essence, the unit tests I want to have in the end are:
Checking presence:
|
@nathanmmiller ooh I get it now! You are totally right, let's go for the behaviour described in your previous comment then. |
…ive-in-prefer-presence-queries
I have reopened this PR at #740 because of a complete mess when rebasing. |
…ence queries
Checks
Changes
Context
Fixes #518 [Well, partially fixes it.]